Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move PNC apply to separate Expr/Pattern variant #7480

Merged
merged 5 commits into from
Jan 8, 2025

Conversation

gamebox
Copy link
Collaborator

@gamebox gamebox commented Jan 7, 2025

No description provided.

@gamebox gamebox requested a review from joshuawarner32 January 7, 2025 23:22
crates/compiler/can/src/desugar.rs Outdated Show resolved Hide resolved
crates/compiler/can/src/desugar.rs Outdated Show resolved Hide resolved
crates/compiler/can/src/expr.rs Outdated Show resolved Hide resolved
crates/compiler/fmt/src/expr.rs Outdated Show resolved Hide resolved
crates/compiler/fmt/src/expr.rs Show resolved Hide resolved
crates/compiler/fmt/src/pattern.rs Outdated Show resolved Hide resolved
crates/compiler/parse/src/expr.rs Outdated Show resolved Hide resolved
@gamebox
Copy link
Collaborator Author

gamebox commented Jan 8, 2025

Ready for re-review

@gamebox gamebox force-pushed the new-pnc-expr-node branch from 7ab2ac2 to 898b3f5 Compare January 8, 2025 20:47
crates/compiler/builtins/roc/Str.roc Show resolved Hide resolved
crates/compiler/fmt/src/pattern.rs Show resolved Hide resolved
crates/compiler/fmt/src/pattern.rs Show resolved Hide resolved
),
Apply(&'a Loc<Pattern<'a>>, &'a [Loc<Pattern<'a>>]),

PncApply(&'a Loc<Pattern<'a>>, Collection<'a, Loc<Pattern<'a>>>),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use a Collection instead of just a slice?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was the whole point of the change. Collections handle trailing comments and have uniform formatting

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, fair. We can leave this for now, and presumably in the future this won't be a problem since Apply is going away.

crates/compiler/parse/src/normalize.rs Show resolved Hide resolved
: f
1(
i,
p, #
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like it's formatted incorrectly to me. Either we should move the comments above the 1 line, or keep it after the closing parens.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to guess that this is consistent with what we do for all collections (lists, tuples, records)...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that's the case, we can make a GitHub issue and try to fix that at some point

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm okay with ignoring for now if we do something like that in order to avoid leaving this in

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking back at this, I don't see how that is not the right formatting, here's the original:

1(i,p#
):f
n

and it is being formatted as:

1(
    i,
    p, #
) : f
n

To me, the comment is obviously meant for the last arg p and since the comment there forces this to be multi-line, that causes the args to be outdented. This feels like the correct formatting to me.


#
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like another case that a comment sneaks inside of parens incorrectly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original is:

1(ts((0
)
#
)
)f:i7f
e

which if I formatted this in my brain (which is hard because this is nonsense):

1(
    ts(
        0,
        #
    ),
)
    f : i7f
e

which is pretty close to what it does:

1(
    ts(
        0,

        #
    ),
)
    f : i7f
e

With migration it should be:

1(
    ts(
        0,

        #
    ),
)(f) : i7f
e

Why?

  1. 1(...) f is a PNC apply as the func in a WS apply. With migration we would turn the outer WS apply in a PNC apply.
  2. The 0 inside of ts(...) has a newline inside the ParensAround, and then another newline in SpacesAfter before the comment. That's the reason for the blank in between after we drop the unneeded ParensAround.

And that's really it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this mostly closely respects the vertical whitespace requested by the user (without being excessive), while indenting and separating terms correctly - as well as making it easy to add a new arg to both 1(..) and ts(..) by adding the trailing commas when outdented.

@lukewilliamboswell lukewilliamboswell merged commit fbf448c into roc-lang:main Jan 8, 2025
16 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants